[i2c,rtl] Predict target clock stretching in HOST mode #21813

hcallahan-lowrisc · 2024-03-04T14:27:57Z

From the commit message :

Previously, the cycle counter for any 'thigh' state would only decrement once
the FSM had observed scl_i = 1'b1 on it's input. When the FSM releases SCL to
create the clock pulse, a minimum of 3 cycles is required to observe this effect
on it's input. (1-cycle output flop, 2-cycle input synchronizer)
This change allows the HOST-mode state machine to proceed if it does not
observe SCL being stretched 4 cycles after releasing SCL to try and create the
next clock pulse. This removes the 3-cycle delay from every 'thigh' state, and
in the absence of clock-stretching, brings the performance of the block in-line
with a user's calculations based on the given timing parameters.
This imposes a minimum 'thigh' of 4 cycles, which may limit the possible
performance of the block when the frequency of clk_i is not significantly
greater than scl.

This approach seemed the simplest way to achieve the intended result, but one cost is the minimum 'thigh' of 4 as described above. I could not come up with a solution that avoided this without greatly increasing complexity, and I'm not even sure one is possible. If we advance on too quickly and reach the "next" tlow, the FSM drives SCL low again, and at this point if the target is stretching we cannot distinguish it from our own driver.

There are some DV gremlins from this change that I have still to resolve (similar to #21765). I am again proposing to fix these up as a follow-up item.

Testing the performance with this change + that from #21765, I see clock frequencies within the 4-cycle possible float as a result of the calculation tlow + t_r + thigh + t_f. This is as good as we can do without stacking the remainders of the quantization calcs, which may be possible in some cases to squeeze out an extra cycle or two.

Closes #18962
Goes toward #18492

hw/ip/i2c/rtl/i2c_pkg.sv

a-will · 2024-03-05T03:49:43Z

hw/ip/i2c/rtl/i2c_fsm.sv

@@ -85,6 +85,8 @@ module i2c_fsm import i2c_pkg::*;
  logic        load_tcount;   // indicates counter must be loaded
  logic [31:0] stretch_idle_cnt; // counter for clock being stretched by target
                                 // or clock idle by host.
+
+  // This bit is active when the FSM is in a state where a TARGET might be trying to stretch


nit: We might want to add a little more so there is no confusion about the role this IP serves. :) For example...

Suggested change

// This bit is active when the FSM is in a state where a TARGET might be trying to stretch

// This bit is active when the FSM is in a state where a TARGET might be trying to stretch the

// clock, preventing the CONTROLLER FSM from continuing.

...or "host FSM," if you prefer. The i2c and SMBus specs actually picked "controller" for this role, though. In SMBus, a "host" is a special kind of controller.

SGTM, I'll do a quick review of the spec's for terminology and try to align with them.

I agree with keeping terminology aligned with the spec. Please don't extend your terminology alignment efforts to beyond the things you're changing, though, because we may have to go through the entire spec and change terminology soon anyway.

Just did a quick flick through the specs, I couldn't see where the I2C spec uses the 'controller' term? It seems to just be Master/Slave. Unless I'm missing something.
Definitely in the medium term we need to update the docs to be unambiguous for both applications.

You don't have the latest with the cleaning of sensitive terms, hehe. Make sure you're looking at 7.0 (or v.7, depending on which moniker you are looking at).

They did a sweep to align with I3C.

Ah yes of course. We still link to the Rev 6. version in the docs. Yet another thing to update!

hw/ip/i2c/rtl/i2c_fsm.sv

marnovandermaas

Thanks for making these changes. I agree with Alex that it would be nice to remove the second state variable if possible. Other than that just a few minor comments.

marnovandermaas · 2024-03-05T14:23:31Z

hw/ip/i2c/rtl/i2c_fsm.sv

      tcount_d = tcount_q - 1'b1;
    end else begin
-      tcount_d = tcount_q;  // pause timer if clock is stretched
+      tcount_d = tcount_q;


Do we still need this else statement? It only covers the case when host and target mode are both disabled in which case we are in the Idle state anyways.

hw/ip/i2c/rtl/i2c_fsm.sv

andreaskurth · 2024-03-07T13:25:04Z

Re DV failures: We should keep the smoke test+seed run by CI green. Currently lc_ctrl_jtag_smoke and chip_sw_i2c_host_tx_rx fail. Did you have a chance to debug these, @hcallahan-lowrisc?

a-will · 2024-03-07T13:43:52Z

hw/ip/i2c/rtl/i2c_fsm.sv

+    if (!rst_ni) begin
+      stretch_predict_cnt_expired <= 1'b0;
+    end else begin
+      if (stretch_idle_cnt == 32'd3) begin


Oh, tClockPulse includes the rise time, so we need to allow for that as well, right?

Suggested change

if (stretch_idle_cnt == 32'd3) begin

if (stretch_idle_cnt == (32'd3 + t_r_i)) begin

I think I agree with you, but I need to think about it a bit more to be sure. That would imply that we document a minimum
value thigh >= t_r + 4 to be safe I think. As before, probably only a downside for very low clk_i input frequencies.

Oh, no, it's still thigh >= 4. Remember, tClockPulse already includes t_r as well. The condition you were satisfying was that the timeout happens before we could possibly change out of the state. In other words...

tStretchCheck < tClockPulse ------------------------------------- t_r + 3 < t_r + t_high ------------------------------------- 3 < t_high

The value 3 comes from the round-trip delay through flops. So, in prose, we wait until the time when we must have seen a 1, and if it happens then, we continue to count out the normal t_high, keeping in mind that we already counted the cycles of delay.

If we don't see a 1, then we wait until we actually see it, then we count out the full t_high to be safe. Technically, this could probably be reduced by the 2 cycles of input delay.

So, t_high must be long enough to accommodate only that round-trip delay. Otherwise, we risk jumping to the next state without ever actually seeing the clock go high.

The value 3 comes from the round-trip delay through flops.

Ah, but I forgot about the glitch filter, which would add yet more delay to the input path. The predictor might expire early, then. If we don't handle it here, the core will still work, though--It just means this fast path might end up rejecting its speculation every time.

a-will

LGTM. Thanks!

(modulo a lint error, hehe)

marnovandermaas · 2024-03-08T10:00:54Z

Kokoro seems unhappy about:
E ARITH_CONTEXT: i2c_fsm.sv:227 Bitlength of arithmetic operation '16'd3 + t_r_i' is self-determined in this context

I guess it means that the 16' is unnecessary because t_r_i is already 16 bits?

marnovandermaas

These changes make sense to me. Bar the Kokoro Lint error.

andreaskurth · 2024-03-08T17:52:15Z

Rebased on master to use the first commit from the now merged PR #21765 and clean failing CI checks (which should have been fixed on master in the meantime).

andreaskurth · 2024-03-11T06:54:24Z

The failing CW310 tests are phony (sourceforge.net problem). The failing chip_sw_i2c_host_tx_rx test, which is part of smoke tests, is real and happens before and after the rebase.

hcallahan-lowrisc · 2024-03-11T19:42:21Z

The failure of the chip_sw_i2c_host_tx_rx test was due to the monitor losing it's lock on the bus again, similar to failures initially encountered in #21765 and patched to be rethought later as part of closing #21887. This time however we were losing the lock around the ACK bit.
I've weakened the checks to remove the setup time here, which fixes that particular test. Assuming the remaining test suite also passes, I think it may be best to expand #21887 to cover these changes too.

andreaskurth · 2024-03-12T09:34:35Z

I've weakened the checks to remove the setup time here, which fixes that particular test. Assuming the remaining test suite also passes, I think it may be best to expand #21887 to cover these changes too.

I'm OK with that; however now the i2c_host_smoke test fails, apparently due to an RTL assertion that fails: (i2c_fsm.sv:1423) [ASSERT FAILED] SclInputGlitch_A.

Previously, the cycle counter for any 'thigh' state would only decrement once the FSM had observed scl_i = 1'b1 on it's input. When the FSM releases SCL to create the clock pulse, a minimum of 3 cycles is required to observe this effect on it's input. (1-cycle output flop, 2-cycle input synchronizer) This change allows the HOST-mode state machine to proceed if it does not observe SCL being stretched (4 + t_r) cycles after releasing SCL to try and create the next clock pulse. This removes the 3-cycle delay from every 'thigh' state, and in the absence of clock-stretching, brings the performance of the block in-line with a user's calculations based on the given timing parameters. This imposes a minimum 'thigh' of 4 cycles, which may limit the possible performance of the block when the frequency of clk_i is not significantly greater than scl. Signed-off-by: Harry Callahan <[email protected]>

andreaskurth

LGTM, thanks for this work, @hcallahan-lowrisc. If CI passes (modulo the currently known unrelated problems), please go ahead with merging this PR.

hcallahan-lowrisc · 2024-03-21T17:06:35Z

CI failures are known-issues, and are unrelated to this PR. Hence merging.

This is no longer necessary now that lowRISC#21813 has been merged. However, due to the 'PERFTHRESHOLD = 0.80' fudge-factor in comparing the exp/obs SCL period in this vseq, the check still passed. This perf check should be made more stringent in the future.

This is no longer necessary now that lowRISC#21813 has been merged. However, due to the 'PERFTHRESHOLD = 0.80' fudge-factor in comparing the exp/obs SCL period in this vseq, the check still passed. This perf check should be made more stringent in the future. Signed-off-by: Harry Callahan <[email protected]>

This is no longer necessary now that #21813 has been merged. However, due to the 'PERFTHRESHOLD = 0.80' fudge-factor in comparing the exp/obs SCL period in this vseq, the check still passed. This perf check should be made more stringent in the future. Signed-off-by: Harry Callahan <[email protected]>

hcallahan-lowrisc requested review from a-will, andreaskurth, marnovandermaas, msfschaffner and alees24 March 4, 2024 14:27

hcallahan-lowrisc requested a review from a team as a code owner March 4, 2024 14:27

hcallahan-lowrisc force-pushed the i2c_predict_clock_stretching branch 3 times, most recently from 694967d to 49e62b9 Compare March 4, 2024 15:11

a-will reviewed Mar 5, 2024

View reviewed changes

marnovandermaas reviewed Mar 5, 2024

View reviewed changes

This was referenced Mar 7, 2024

[i2c,rtl] Clock stretch causes timing mismatch #18962

Closed

[i2c] Clock speed mismatch #18492

Closed

a-will reviewed Mar 7, 2024

View reviewed changes

hcallahan-lowrisc force-pushed the i2c_predict_clock_stretching branch 4 times, most recently from da9327e to 223e6a4 Compare March 7, 2024 18:13

a-will approved these changes Mar 7, 2024

View reviewed changes

hcallahan-lowrisc force-pushed the i2c_predict_clock_stretching branch 2 times, most recently from 41b117b to 3e171f0 Compare March 7, 2024 19:27

marnovandermaas approved these changes Mar 8, 2024

View reviewed changes

hcallahan-lowrisc force-pushed the i2c_predict_clock_stretching branch from 3e171f0 to fad9c33 Compare March 8, 2024 14:30

andreaskurth force-pushed the i2c_predict_clock_stretching branch from fad9c33 to fe427b2 Compare March 8, 2024 17:51

hcallahan-lowrisc force-pushed the i2c_predict_clock_stretching branch from fe427b2 to 0c8672b Compare March 11, 2024 19:30

hcallahan-lowrisc force-pushed the i2c_predict_clock_stretching branch from 0c8672b to 1deaee5 Compare March 15, 2024 16:15

hcallahan-lowrisc force-pushed the i2c_predict_clock_stretching branch from 1deaee5 to 12ddea4 Compare March 19, 2024 14:06

andreaskurth approved these changes Mar 19, 2024

View reviewed changes

hcallahan-lowrisc merged commit 689a163 into lowRISC:master Mar 21, 2024
27 of 32 checks passed

hcallahan-lowrisc deleted the i2c_predict_clock_stretching branch March 22, 2024 13:54

This was referenced Mar 26, 2024

[i2c] D2(S) Signoff #20971

Closed

[i2c] Implement a digital 50ns Glitch/Spike Filter #22280

Open

hcallahan-lowrisc mentioned this pull request Mar 27, 2024

[i2c,dv] Remove +3 correction for coerced period in host_perf_vseq #22286

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[i2c,rtl] Predict target clock stretching in HOST mode #21813

[i2c,rtl] Predict target clock stretching in HOST mode #21813

hcallahan-lowrisc commented Mar 4, 2024 •

edited

Loading

a-will Mar 5, 2024

hcallahan-lowrisc Mar 5, 2024

andreaskurth Mar 7, 2024 •

edited

Loading

hcallahan-lowrisc Mar 7, 2024

a-will Mar 7, 2024 •

edited

Loading

hcallahan-lowrisc Mar 7, 2024

marnovandermaas left a comment

marnovandermaas Mar 5, 2024

andreaskurth commented Mar 7, 2024

a-will Mar 7, 2024

hcallahan-lowrisc Mar 7, 2024

a-will Mar 7, 2024 •

edited

Loading

a-will Mar 11, 2024

a-will left a comment •

edited

Loading

marnovandermaas commented Mar 8, 2024

marnovandermaas left a comment

andreaskurth commented Mar 8, 2024

andreaskurth commented Mar 11, 2024

hcallahan-lowrisc commented Mar 11, 2024

andreaskurth commented Mar 12, 2024

andreaskurth left a comment

hcallahan-lowrisc commented Mar 21, 2024

	// This bit is active when the FSM is in a state where a TARGET might be trying to stretch
	// This bit is active when the FSM is in a state where a TARGET might be trying to stretch the
	// clock, preventing the CONTROLLER FSM from continuing.

	if (stretch_idle_cnt == 32'd3) begin
	if (stretch_idle_cnt == (32'd3 + t_r_i)) begin

[i2c,rtl] Predict target clock stretching in HOST mode #21813

[i2c,rtl] Predict target clock stretching in HOST mode #21813

Conversation

hcallahan-lowrisc commented Mar 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andreaskurth Mar 7, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

a-will Mar 7, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marnovandermaas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andreaskurth commented Mar 7, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

a-will Mar 7, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

a-will left a comment • edited Loading

Choose a reason for hiding this comment

marnovandermaas commented Mar 8, 2024

marnovandermaas left a comment

Choose a reason for hiding this comment

andreaskurth commented Mar 8, 2024

andreaskurth commented Mar 11, 2024

hcallahan-lowrisc commented Mar 11, 2024

andreaskurth commented Mar 12, 2024

andreaskurth left a comment

Choose a reason for hiding this comment

hcallahan-lowrisc commented Mar 21, 2024

hcallahan-lowrisc commented Mar 4, 2024 •

edited

Loading

andreaskurth Mar 7, 2024 •

edited

Loading

a-will Mar 7, 2024 •

edited

Loading

a-will Mar 7, 2024 •

edited

Loading

a-will left a comment •

edited

Loading